Perhaps we can feed them to the treasury, or explicitly burn them (which is effectively what is happening now).

I think it's better leave this up to the validator/nominator. If one wants to donate to treasury, it can just use RewardDestination::Account(treasury_address).

I think we need to send those into RewardRemainder otherwise the inflection rate of a chain will be unstable.

@xlc The RewardRemainder is used to handle how much rewards go into treasury vs validators, so I don't think it's suitable to use it here. https://github.com/paritytech/substrate/blob/master/frame/staking/src/lib.rs#L2817

Staking payouts only deal with "targeted" / "theoretical" inflation rate. The "practical" inflation rate will always remain unstable -- each year, there will always be some users who send funds to unspendable addresses by mistakes, or lose their private keys. RewardDestination::None is no different than RewardDestination::Address(an_unspendable_address), but compared with the later, we avoid those unnecessary processing of an unusable address, and make balances' total issuance number better tracking the accurate total issuance.

No there is a major difference. Say initial issuance is 100 and inflection rate is 10% per annual. On next year we should have 110 and one more year is 121.

But some validators choose to burn the rewards so 1 year after could be 105 and now the inflection on second year will be 10.5 instead of 11.

Inaccessible token is different than nonexist token.

If not burning token is a requirement for staking, I would suggest we add this to staking module's audit requirements and fix things accordingly. Right now there are so many ways an "attacker" can deliberately burn tokens. For example:

• Staking module's payout period can be set to longer to the bonding period (and it's indeed the case on Polkadot). So a validator/nominator can generate some rewards and leave them unclaimed. Then unbond, and claim rewards when all the bonding is cleared. The reward will be burned but not sent to the payee, because Self::bond at that time will return None.
• One can also generate a large number of nominators, and make it so that each day's reward is less than the existence balance. The nominator then set the reward destination to a new account and claim rewards every day. The reward will be burned, because while the sum might be large enough, each individual payment will be less than the existence balance.

Those are just two methods that immediately pop up in my head, for current methods of burning tokens. If we don't want them to happen we'd probably need to make sure they're fixed.

That said, I still don't believe those burning token instances are of anything bad, given tokens can be made inaccessible by accidents anyway, and as long as we have good incentives in place so that unless people have special needs, they wouldn't choose burning.

If your argument to the above existing burning token problem is that "they happen really rarely", then you can consider "RewardDestination::None" to be rare as well. If your argument is that burning token is a thing that should never occur, then you need to be wary about existing other ways that tokens can be burned, audit them, and mitigate them accordingly.

Just want to write down all existing methods I can think of in case we really want to audit all the burning token problems. Another really simple method to burn tokens right now is just -- not to claim the reward. After the payout deadline ends, the reward will be removed from staking module, and burned.

If we really want to "fix" them, then one way is to have some reward payouts at on_finalize when they approach the deadline (or instead send them to treasury). No matter what, requiring not burning any tokens will add some extra processing burden, compared with our current model of allowing some tokens to be burned.

(But as I already said above, I'm still not convinced why burning token is an issue at all.)

@sorpaas yeah you are right that currently pallet-staking already allow burning rewards so maybe it is not an issue to introduce another way of burning the rewards.

This simply means for projects that does not allow burning token, they cannot use pallet-staking. Will be helpful to make this explicit somewhere.

For Polkadot / Kusama, it will be up to the token economic people to decide if it is safe to have this property.

My 2 cents are as follows:

We can and should work toward making staking keep up with the inflation rate. The example of "Someone can send tokens to an inaccessible account" is not the same "not minting them in the first place". In the former, the tokens are minted, and while practically they may not be usable (no private key), theoretically they are in the system and staking has done its job correctly (also to be the devils advocate, the assumption based on which they are not inaccessible might change due to a runtime upgrade -- or quantum computing :P). Whereas with not minting them in the first place, we're obviously not keeping with the goal, and it is a shortcoming of staking.

We should: Allow unbalance handlers for cases where a reward can be lost, and configure what should happen to them. The chain does not care? Then they can explicitly burn them. They do? Then they can send them to the treasury. While it won't be easy, I believe it is possible to do this. At least for unclaimed rewards, and rewards that are forgotten due to unbonding, it is surely possible. @thiolliere maybe you can pick this up? :)

This might make the interface of staking a bit verbose if we are to have one handler for each case, but I think it is worthwhile, and staking is a complicated pallet. Using it correctly trumps making it simple to use.

@sorpaas gave one example of what type of fixes we would need, to which I very much agree:

If we really want to "fix" them, then one way is to have some reward payouts at on_finalize when they approach the deadline (or instead send them to treasury). No matter what, requiring not burning any tokens will add some extra processing burden, compared with our current model of allowing some tokens to be burned.

Also, this PR should ideally adhere to the same principle. If a reward is going to oblivion, it should be configurable where it is going to, which brings me back to my initial comment:

Shan't we make it configurable what to do with unused rewards? Perhaps we can feed them to the treasury, or explicitly burn them (which is effectively what is happening now).

Although, given that we have the issue to follow-up, we can merge this as-is and fix all the reward-leaks later. But I'd be happy to see this one fixed here as well.

I think the best way to move forward -- regarding the burn issue, is to refactor balances and other modules to properly track coins burned. Not merging this PR does not do us anything good -- we already have too many ways where coins can be burned already.

I can approve this change as it is now, given that it will also not go into polkadot. But this is by all means not the end of the discussion around this.

Rewards not being minted is a non-issue. If a staker wishes to not collect their rewards, that's their business. Nowhere in the code or social contract does Polkadot state that rewards need to be collected.

After the payout deadline ends, the reward will be removed from staking module, and burned.

Tokens are burned (or not). Rewards are merely ignored (or collected). Rewards != tokens. A reward is the right to mint some tokens. As a staker, you need not decide to exercise that right.

bot merge

Checks failed; merge aborted.

Only check failing is continuous-integration/gitlab-cargo-deny which we cannot fix in this PR. So I guess I have to manually merge this.